Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

vm: add import assertion support #37176

Merged
merged 1 commit into from
Mar 23, 2021
Merged

vm: add import assertion support #37176

merged 1 commit into from
Mar 23, 2021
+40 −4

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Feb 1, 2021

Just a draft until our V8 is new enough to support this.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 1, 2021
@devsnek devsnek marked this pull request as draft February 1, 2021 22:22
@devsnek devsnek added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 1, 2021
cjihrig
cjihrig reviewed Feb 1, 2021
View reviewed changes
doc/api/vm.md Outdated Show resolved Hide resolved
addaleax
addaleax approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (with a comment to avoid crashing in cases of prototype pollution addressed)

src/module_wrap.cc Show resolved Hide resolved
@devsnek devsnek marked this pull request as ready for review March 23, 2021 00:21
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36921/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36928/

@devsnek
vm: add import assertion support
26eed3e 
PR-URL: nodejs#37176
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@devsnek devsnek merged commit 26eed3e into nodejs:master Mar 23, 2021
@devsnek devsnek deleted the vm-import-assertion branch March 23, 2021 16:04
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
@devsnek @ruyadorno
vm: add import assertion support
4f354b0 
PR-URL: #37176
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@ruyadorno
Copy link
Member

hi @devsnek this commit is breaking make cctest in v15.x-staging - should it be backported to v15.x?

@devsnek
Copy link
Member Author

devsnek commented Mar 30, 2021

@ruyadorno nope! its stuck to the v8 version in master 😄

@BethGriggs BethGriggs mentioned this pull request Apr 12, 2021
Merged
@aduh95 aduh95 mentioned this pull request Oct 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig left review comments

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@devsnek @nodejs-github-bot @ruyadorno @addaleax @targos @cjihrig